Skip to content

fix: rename deduplication tool to memory_deduplicate to avoid collision#441

Closed
Displayer226 wants to merge 2 commits intoCortexReach:masterfrom
Displayer226:fix/memory-compact-conflict
Closed

fix: rename deduplication tool to memory_deduplicate to avoid collision#441
Displayer226 wants to merge 2 commits intoCortexReach:masterfrom
Displayer226:fix/memory-compact-conflict

Conversation

@Displayer226
Copy link
Copy Markdown
Contributor

@Displayer226 Displayer226 commented Apr 1, 2026

Summary

This PR resolves the naming conflict for the memory_compact tool by renaming the metadata-based deduplication version to memory_deduplicate.

Motivation

Fixes #431.
This is offered as an alternative to PR #433.

While #433 proposes removing one of the implementations, this PR argues that both are essential for a complete memory lifecycle:

  1. Deduplication (memory_deduplicate): A lightweight, metadata-based tool in src/tools.ts used for structural hygiene (archiving identical or redundant facts).
  2. Summarization (memory_compact): A heavyweight, LLM-based tool in index.ts used for increasing knowledge density by merging related memories into refined entries.

By renaming instead of removing, we preserve both functionalities for the user.

Key Changes

  • Renamed registerMemoryCompactTool to registerMemoryDeduplicateTool in src/tools.ts.
  • Changed the exposed tool name from memory_compact to memory_deduplicate.
  • Updated registerAllMemoryTools to reflect the new name.

Testing

  • Verified that both tools can now be registered simultaneously without gateway startup errors.
  • Manual test of memory_deduplicate confirmed it correctly identifies and archives duplicate metadata entries.

@rwmjhb rwmjhb mentioned this pull request Apr 1, 2026
Closed
3 tasks
@rwmjhb
Copy link
Copy Markdown
Collaborator

rwmjhb commented Apr 1, 2026

Review: REQUEST-CHANGES

The rename approach is the right call — both tools serve different purposes (dedup vs progressive compaction), so they should coexist under distinct names. Closing #433 in favor of this direction.

Must fix:

  1. Governance test is broken — test/memory-governance-tools.test.mjs:157 still asserts memory_compact is registered. After the rename, this test fails with AssertionError: tool memory_compact should be registered. This file is also not in the default npm test script, so CI won't catch it.

  2. Parameter style mismatch — the renamed memory_deduplicate uses camelCase dryRun (default: preview), while the surviving memory_compact uses snake_case dry_run (default: write). A caller using { dryRun: true } against memory_compact silently gets write behavior instead of preview.

Worth considering:

  • The rename is a breaking change for any existing user or LLM agent calling memory_compact expecting dedup behavior — consider documenting the migration in the PR description or a changelog note.
  • Branch is behind main — please rebase.

@Displayer226 Displayer226 force-pushed the fix/memory-compact-conflict branch from eb1fb62 to 2fdec0e Compare April 1, 2026 20:24
@Displayer226
Copy link
Copy Markdown
Contributor Author

Displayer226 commented Apr 1, 2026

Hello !

Thanks for the review! I've updated the PR to address all the 'Must fix' items and rebased on the latest master:

  • Parameter Standardization: Standardized memory_compact parameters to camelCase (dryRun, minAgeDays, similarityThreshold) and
    set dryRun: true as the default for safety, matching the memory_deduplicate style.
  • Fixed Governance Test: Updated test/memory-governance-tools.test.mjs to target the correct tool name.
  • Changelog: Added a note in CHANGELOG.md regarding the breaking rename and parameter changes for version 1.1.0-beta.11.
  • Rebase & CI Cleanup: Rebased on master and fixed two existing test regressions in plugin-manifest-regression and
    session-summary-before-reset caused by recent changes to selfImprovement hook defaults.
  • Extra Verification: Added a new check in test/plugin-manifest-regression.mjs to ensure both tools remain correctly registered with their new camelCase interface and safe defaults going forward.

The full test suite is now passing locally (npm test).

@Displayer226 Displayer226 force-pushed the fix/memory-compact-conflict branch from 2fdec0e to 2bfccf3 Compare April 1, 2026 20:30
AliceLJY
AliceLJY approved these changes Apr 2, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@AliceLJY AliceLJY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — clean rename + parameter standardization, addresses all prior review feedback.

Verified:

  • memory_compact (progressive summarization) and memory_deduplicate (metadata dedup) now coexist with distinct names
  • Parameters consistently use camelCase across both tools
  • dryRun defaults to true (safe preview mode) for both tools
  • Governance test updated to target memory_deduplicate
  • Plugin manifest regression test extended with assertions for both tools' parameter schemas
  • CHANGELOG clearly documents the breaking changes
  • selfImprovement: { enabled: false } additions fix unrelated test regressions from upstream

Minor note (non-blocking): In plugin-manifest-regression.mjs, the updated registerTool mock silently drops calls where toolOrFactory is a function but meta is undefined (neither if branch matches). Original code would have thrown on meta.name. Since this is test-only, low risk, but worth noting in case a future tool registration path hits that gap.

Assigning to @rwmjhb for final merge decision.

@app3apps ChatGPT Codex Connector
Copy link
Copy Markdown

app3apps commented Apr 3, 2026

Current status from my side: I still think this PR is logically fine, but it is not mergeable as-is because master has moved and there is now a real conflict in test/plugin-manifest-regression.mjs.

I checked the merge locally. This looks like a test-only conflict, not a new product/code regression.

The resolution should keep both parts of intent:

  • keep the new memory_compact / memory_deduplicate registration + camelCase schema assertions from this PR
  • keep this PR's explicit selfImprovement: { enabled: false } setup in the affected test harnesses

With that setup, the command:new assertions in this file should stay aligned with selfImprovement being disabled in the test config, rather than copying over the newer default-on assertion from master unchanged.

I also verified locally that after resolving the conflict that way, node test/plugin-manifest-regression.mjs passes.

So my recommendation is: please rebase, resolve test/plugin-manifest-regression.mjs with that shape, and then this should be ready to merge.

@Displayer226
fix: rename deduplication tool to memory_deduplicate to avoid collision
f032e6b 
- Rename metadata-based deduplication tool to memory_deduplicate to resolve conflict with progressive summarization
- Standardize all memory management tools to use camelCase parameters (dryRun, minAgeDays, similarityThreshold)
- Set dryRun: true as the safe default for all compaction/deduplication tools
- Fix broken memory-governance, plugin-manifest, and session-summary tests
- Add explicit verification for standardized tool registration in manifest regression tests
- Document breaking changes in CHANGELOG.md
@Displayer226
test: update plugin manifest regression tests to expect command:new h…
408e73a 
…ook to be undefined when selfImprovement is disabled
@Displayer226 Displayer226 force-pushed the fix/memory-compact-conflict branch from 2bfccf3 to 408e73a Compare April 3, 2026 03:57
@rwmjhb
Copy link
Copy Markdown
Collaborator

rwmjhb commented Apr 4, 2026

Review: rename deduplication tool to memory_deduplicate to avoid collision

Tool name collision causing ERROR on every gateway start is a real problem worth fixing.

Must Fix

1. No test validates the rename or collision fix

The PR changes the tool name but adds no test asserting the new name works or that the collision is resolved. A minimal test: register both tools, verify no error.

2. Rebase needed — build is red (stale base)

plugin-manifest-regression.mjs failure is pre-existing.

Worth discussing

  • Breaking change: renaming memory_compact to memory_deduplicate breaks any existing consumer (LLM agents, scripts) that references the old name. Consider: is the alternative approach in PR fix: remove duplicate memory_compact tool registration #433 (remove the duplicate registration instead of renaming) a better fix? That preserves the existing tool name.

@rwmjhb rwmjhb closed this Apr 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AliceLJY AliceLJY AliceLJY approved these changes

Assignees

@rwmjhb rwmjhb

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

plugin tool name conflict (memory-lancedb-pro): memory_compact — 工具重复注册导致 ERROR

4 participants

@Displayer226 @rwmjhb @app3apps @AliceLJY